[SPARK-20159][SPARKR][SQL] Support all catalog API in R by felixcheung · Pull Request #17483 · apache/spark

felixcheung · 2017-03-30T16:28:06Z

What changes were proposed in this pull request?

Add a set of catalog API in R

"currentDatabase",
"listColumns",
"listDatabases",
"listFunctions",
"listTables",
"recoverPartitions",
"refreshByPath",
"refreshTable",
"setCurrentDatabase",

https://github.com/apache/spark/pull/17483/files#diff-6929e6c5e59017ff954e110df20ed7ff

How was this patch tested?

manual tests, unit tests

felixcheung · 2017-03-30T16:33:09Z

+#' @param tableName a name of the table.
+#' @param path the path of files to load.
+#' @param source the name of external data source.
+#' @param schema the schema of the data for certain data source.


this is added to createExternalTable (won't work with json source otherwise)
everything else is simply moved as-is from SQLContext.R

SparkQA · 2017-03-30T17:05:36Z

Test build #75393 has finished for PR 17483 at commit e4808b6.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

shivaram · 2017-03-30T18:30:02Z

cc @gatorsmile for any SQL specific inputs

@felixcheung I will take a look at this later today. Meanwhile in the PR description can you note down what are the new functions being added in this change ?

gatorsmile · 2017-03-30T21:44:20Z

+  dataFrame(sdf)
+}
+
+createExternalTable <- function(x, ...) {


Just FYI, createExternalTable is deprecated. See the PR: #16528

Let me make the corresponding changes in SQLContext too.

yes, I'm aware. I'm not sure if we need to make changes to SQLContext - schema is required for json source but it's ok in Scala to use createTable instead.

It makes sense to add in R here though because

there is no createTable method (hmm, reviewing that PR you reference, maybe we should add it)

createTable just sounds too generic and too much like many existing R methods (in R, table is everywhere!), that I wasn't sure it's a good idea to add in R

createExternalTable since 2.0 is decoupled from SQLContext or SparkSession - it doesn't take either as parameter and it's calling on catalog

I agree that createTable sounds very general, but I dont think its used by base R or any popular R package ?

createExternalTable is misleading now, because the table could be managed if users did not provide the value of path. Thus, we decided to rename it.

right, I was just concerned that with data.table, read.table etc, table == data.frame in R as supposed to hive table or managed table, which could be fairly confusing.
anyway, I think I'll follow up with a PR for createTable but as of now path is optional for createExternalTable, even though it's potentially misleading, it does work now.

If you drop a managed table both data and meta data will be deleted if you drop an external table only metadata is deleted, external table is a way to protect data against accidental drop commands.

Thus, it is a pretty important concept. It could be either Hive or Spark native one.

felixcheung · 2017-03-31T03:33:19Z

updated PR description!

felixcheung · 2017-03-31T03:45:42Z

btw, @gatorsmile it looks like listColumns should throw NoSuchTableException and/or NoSuchDatabaseException instead of AnalysisException here

SparkQA · 2017-03-31T03:51:08Z

Test build #75417 has finished for PR 17483 at commit 5ab5834.

This patch fails R style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-31T04:28:53Z

Test build #75418 has finished for PR 17483 at commit 28195b9.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-31T05:49:29Z

Test build #75420 has finished for PR 17483 at commit 9c768ae.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-31T06:33:18Z

Test build #75422 has finished for PR 17483 at commit 5093891.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

shivaram

Some comments. Change looks good otherwise

shivaram · 2017-03-31T21:57:09Z

  createOrReplaceTempView(df, "table1")
  expect_equal(length(tableNames()), 1)
-  tables <- tables()
+  tables <- listTables()


is tables() deprecated now ?

right, there are some differences of the output (most notability catalog.listTables returns a Dataset<Table> - but I'm converting that into a DataFrame anyway), and I thought list* would be more consistent with other methods like listColumn(), listDatabases()

> head(tables("default")) database tableName isTemporary 1 default json FALSE Warning message: 'tables' is deprecated. Use 'listTables' instead. See help("Deprecated") > head(listTables("default")) name database description tableType isTemporary 1 json default <NA> EXTERNAL FALSE

If you think it makes sense, we could make tables an alias of listTables - it's going to call slightly different code on the Scala side and there are new columns and one different column name being returned.

shivaram · 2017-03-31T21:59:40Z

+               "org.apache.spark.sql.catalyst.expressions.Abs")
+  expect_error(listFunctions("foo_db"),
+               "Error in listFunctions : analysis error - Database 'foo_db' does not exist")
+})


We dont have tests for recoverPartitions refreshByPath and refreshTable ?

sharp eyes :) I was planning to add tests.

I tested these manually, but the steps are more involved and these are only thin wrappers in R I think we should defer to scala tests.

shivaram · 2017-03-31T22:01:34Z

+  dataFrame(sdf)
+}
+
+createExternalTable <- function(x, ...) {


I agree that createTable sounds very general, but I dont think its used by base R or any popular R package ?

gatorsmile · 2017-04-01T06:00:31Z

    first <- strsplit(msg[2], "\r?\n\tat")[[1]][1]
    stop(paste0(rmsg, "analysis error - ", first), call. = FALSE)
+  } else
+    if (any(grep("org.apache.spark.sql.catalyst.analysis.NoSuchDatabaseException: ", stacktrace))) {


Yes. I knew it. See the JIRA: https://issues.apache.org/jira/browse/SPARK-19952. @hvanhovell plans to remove it in 2.3.0

…s(), test for recoverPartitions/refresh*

SparkQA · 2017-04-01T18:44:10Z

Test build #75447 has finished for PR 17483 at commit 3c66930.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

felixcheung · 2017-04-01T22:52:42Z

@gatorsmile I added a line about recoverPartitions, I think we should also be more clear in other language bindings?

Also open https://issues.apache.org/jira/browse/SPARK-20188

SparkQA · 2017-04-01T23:23:43Z

Test build #75451 has finished for PR 17483 at commit 2aea0cb.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-04-01T23:28:48Z

Test build #75452 has finished for PR 17483 at commit aff13a8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

felixcheung · 2017-04-02T18:59:45Z

merged to master. thanks for the review!

…createExternalTable ## What changes were proposed in this pull request? Following up on apache#17483, add createTable (which is new in 2.2.0) and deprecate createExternalTable, plus a number of minor fixes ## How was this patch tested? manual, unit tests Author: Felix Cheung <felixcheung_m@hotmail.com> Closes apache#17511 from felixcheung/rceatetable.

MaxGekk · 2020-11-28T14:26:50Z

-#' @note tables since 1.4.0
-tables.default <- function(databaseName = NULL) {
-  sparkSession <- getSparkSession()
-  jdf <- callJStatic("org.apache.spark.sql.api.r.SQLUtils", "getTables", sparkSession, databaseName)


if I am not mistaken, the method getTables() is not used any more in R. Can we remove it from r.SQLUtils:

spark/sql/core/src/main/scala/org/apache/spark/sql/api/r/SQLUtils.scala

Lines 219 to 226 in e8982ca

def getTables(sparkSession: SparkSession, databaseName: String): DataFrame = {

databaseName match {

case n: String if n != null && n.trim.nonEmpty =>

Dataset.ofRows(sparkSession, ShowTablesCommand(Some(n), None))

case _ =>

Dataset.ofRows(sparkSession, ShowTablesCommand(None, None))

}

}

? cc @HyukjinKwon

Yeah, I saw the PR. LGTM

### What changes were proposed in this pull request? Remove the unused method `getTables()` from `r.SQLUtils`. The method was used before the changes #17483 but R's `tables.default` was rewritten using `listTables()`: https://github.com/apache/spark/pull/17483/files#diff-2c01472a7bcb1d318244afcd621d726e00d36cd15dffe7e44fa96c54fce4cd9aR220-R223 ### Why are the changes needed? To improve code maintenance, and remove the dead code. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? By R tests. Closes #30527 from MaxGekk/remove-getTables-in-r-SQLUtils. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

Move catalog-base method into a new file, add catalog APIs, tests

e4808b6

felixcheung commented Mar 30, 2017

View reviewed changes

gatorsmile reviewed Mar 30, 2017

View reviewed changes

fix test failure

5ab5834

off by one char

28195b9

improve exception handling that fixes the test too

9c768ae

address deprecation

5093891

shivaram reviewed Mar 31, 2017

View reviewed changes

gatorsmile reviewed Apr 1, 2017

View reviewed changes

make tables() alias instead of deprecate, error handling in tableName…

3c66930

…s(), test for recoverPartitions/refresh*

fix test

2aea0cb

add doc links

aff13a8

asfgit closed this in 93dbfe7 Apr 2, 2017

felixcheung mentioned this pull request Apr 2, 2017

[SPARK-20195][SPARKR][SQL] add createTable catalog API and deprecate createExternalTable #17511

Closed

MaxGekk reviewed Nov 28, 2020

View reviewed changes

MaxGekk mentioned this pull request Nov 28, 2020

[MINOR][SQL] Remove getTables() from r.SQLUtils #30527

Closed

felixcheung deleted the rcatalog branch February 6, 2021 23:49

	def getTables(sparkSession: SparkSession, databaseName: String): DataFrame = {
	databaseName match {
	case n: String if n != null && n.trim.nonEmpty =>
	Dataset.ofRows(sparkSession, ShowTablesCommand(Some(n), None))
	case _ =>
	Dataset.ofRows(sparkSession, ShowTablesCommand(None, None))
	}
	}

Conversation

felixcheung commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 30, 2017

Uh oh!

shivaram commented Mar 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixcheung commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Mar 31, 2017

Uh oh!

SparkQA commented Mar 31, 2017

Uh oh!

SparkQA commented Mar 31, 2017

Uh oh!

SparkQA commented Mar 31, 2017

Uh oh!

shivaram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung Apr 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 1, 2017

Uh oh!

felixcheung commented Apr 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Apr 1, 2017

Uh oh!

SparkQA commented Apr 1, 2017

Uh oh!

felixcheung commented Apr 2, 2017

Uh oh!

MaxGekk Nov 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Mar 30, 2017 •

edited

Loading

felixcheung Mar 31, 2017 •

edited

Loading

felixcheung commented Mar 31, 2017 •

edited

Loading

felixcheung commented Mar 31, 2017 •

edited

Loading

felixcheung Apr 1, 2017 •

edited

Loading

felixcheung commented Apr 1, 2017 •

edited

Loading

MaxGekk Nov 28, 2020 •

edited

Loading